-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Doppler ICP algorithm in registration pipeline and dopplers field in PointCloud #5220
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 21 files at r1, 7 of 8 files at r2, all commit messages.
Reviewable status: 20 of 21 files reviewed, 5 unresolved discussions (waiting on @heethesh and @reyanshsolis)
cpp/open3d/geometry/PointCloud.h
line 451 at r2 (raw file):
std::vector<Eigen::Vector3d> colors_; /// Doppler velocity of points. std::vector<double> dopplers_;
Adding a new hard-coded field to the geometry::PointCloud
class is not very ideal, as this field is only used in the DICP algorithm. The new t::geometry::PointCloud
handles this problem since we can attach arbitrary property (as tensor) to the class. I recommend moving the implementation to the t::geometry::PointCloud
class. Same for the pipeline, we should move to t::pipeline
.
You may check out cpp/open3d/t/pipelines/registration/Registration.h
for reference. For simplicity, we may focus on the CPU implementation for now. @reyanshsolis can answer any questions that you may have regarding the tensor-based registration.
cpp/open3d/io/PointCloudIO.h
line 150 at r2 (raw file):
const WritePointCloudOption ¶ms); bool ReadPointCloudFromXYZD(const std::string &filename,
Similarly, these functions can be moved to t::io
examples/cpp/RegistrationDopplerICP.cpp
line 97 at r2 (raw file):
std::shared_ptr<geometry::PointCloud> target = open3d::io::CreatePointCloudFromFile(argv[2]); if (source == nullptr || target == nullptr) {
it would be the best to use one of the example datasets, if the user does not specify any path, e.g. http://www.open3d.org/docs/latest/tutorial/data/index.html#demoicppointclouds
examples/cpp/RegistrationDopplerICP.cpp
line 146 at r2 (raw file):
return 0; }
We also need to add a C++ unit test (cpp/tests/t/pipelines/registration/Registration.cpp
) before migrating to the t::
namespace.
- Use the example dataset
- Run the DICP for a fixed number of iterations, using fixed parameter
- Assert the output to be close to some numerical values
This unit test will make sure that, after your code is migrated to the t::
namespace, the numerical values remain the same.
examples/python/pipelines/doppler_icp.py
line 55 at r2 (raw file):
args = parser.parse_args() source = o3d.io.read_point_cloud(args.source)
same, it would be the best to use one of the example datasets, if the user does not specify any path, e.g. http://www.open3d.org/docs/latest/tutorial/data/index.html#demoicppointclouds
@yxlao Agreed, my main concern was adding the Regarding the dataset for tests, the Doppler ICP algorithm requires point clouds from FMCW sensors with per-point velocities. We have a couple of sample datasets generated in CARLA here. I can pick a small subset from this for the examples. Where do I add the examples? Is that another repository you maintain or LFS? |
Practically, I found that the runtime with legacy registration pipeline (with OpenMP) was the best, followed by tensor GPU and the CPU tensor version being extremely slow. Do you have any plans to support additional variable/non hard-coded fields (keeping |
The tensor-based implementation performance is better than legacy, but it is a bit more sensitive to the search radius parameters. So, given the parameters are tuned Tensor GPU >> Eigen/Legacy CPU ~ Tensor CPU. Tensor-based modules are the new modules, and we intend to keep improving their performance of the same, whereas the legacy might get depreciated in the future. I can help you migrate the same quickly to the tensor-based pipeline. |
The most likely reason is in the implementation. If we use a for loop with repetitive tensor slicing and indexing, it will be slow. For best performance, we should use Tensors:
In the following benchmark, the tensor-based ICP is faster than legacy: $ make benchmarks -j && ./bin/benchmarks --benchmark_filter=".*ICP.*"
-------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
-------------------------------------------------------------------------------------
BenchmarkICPLegacy/PointToPlane / CPU 44.4 ms 40.8 ms 20
BenchmarkICPLegacy/PointToPoint / CPU 104 ms 104 ms 8
BenchmarkICP/"CPU:0" PointToPoint_Float32 31.4 ms 31.2 ms 23
BenchmarkICP/"CPU:0" PointToPoint_Float64 34.2 ms 31.1 ms 22
BenchmarkICP/"CPU:0" PointToPlane_Float32 33.4 ms 33.2 ms 22
BenchmarkICP/"CPU:0" PointToPlane_Float64 33.2 ms 33.1 ms 22
BenchmarkICP/"CPU:0" ColoredICP_Float32 39.9 ms 39.9 ms 16
BenchmarkICP/"CPU:0" ColoredICP_Float64 82.4 ms 69.3 ms 12
BenchmarkICP/"CUDA:0" PointToPoint_Float32 15.0 ms 15.0 ms 41
BenchmarkICP/"CUDA:0" PointToPoint_Float64 24.1 ms 24.1 ms 28
BenchmarkICP/"CUDA:0" PointToPlane_Float32 9.07 ms 9.07 ms 88
BenchmarkICP/"CUDA:0" PointToPlane_Float64 15.4 ms 15.4 ms 46
BenchmarkICP/"CUDA:0" ColoredICP_Float32 11.3 ms 11.3 ms 62
BenchmarkICP/"CUDA:0" ColoredICP_Float64 20.4 ms 20.4 ms 34 |
Okay, let me create a PR with the kernels I implemented in the tensor pipeline for your review. |
@heethesh if you want, we can have a zoom meeting too. So, that I can help you migrate the kernels easily. |
@reyanshsolis I have a working implementation of CPU/GPU kernels in the tensor pipeline implemented already. I'll try to get the PR for it up soon, and we can probably set up a call to help optimize it after a first review. |
@heethesh The tensor-based implementation uses radius search, then takes apply the distance threshold, as compared to legacy using knn search, which makes tensor highly sensitive to radius search parameter ( Voxel size 0.02max_correspondence_distance 0.05 (generally it sould be around 1.4x voxel size)
max_correpondence_distance 0.1 (very poor init)
So, given parameters in an acceptable range, the tensor-based API provided better performance and also a lot of flexibility and control over performance, such as multi-scale ICP, custom robust kernels for outlier rejection, and real-time update of intermediate results through lambda function. |
@reyanshsolis @yxlao please see my new PR #5237 |
Appreciate the contribution and effort. But we will be merging new features to the tensor-based pipeline in the interest of a stable tensor API and depreciating the eigen-based legacy api. |
We would like to contribute the implementation of our Doppler ICP algorithm for point clouds captured by FMCW LiDARs. This is the implementation of the following paper:
DopplerICP
algorithm inopen3d::pipelines::registration
.dopplers
field inopen3d::geometry::PointCloud
class.converged
andnum_iterations
inRegistrationResult
.This change is